Add a vertical toolbar for zoom out mode #60123
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +476 B (+0.03%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in b1b5d7a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9578571343
|
|
I'm going to focus first on the other items in #60124 before continuing this exploration |
MaggieCabrera
force-pushed
the
try-vertical-toolbar-zoomed-out
branch
from
437dc3c
to
5633e87
Compare
MaggieCabrera
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
I think it would be nice if the toolbar stuck to the top of the screen. |
mirka
reviewed
May 16, 2024
MaggieCabrera
force-pushed
the
try-vertical-toolbar-zoomed-out
branch
from
e4c4303
to
259597b
Compare
|
I addressed the review of the component's changes. Do we think this is a good starting point for the toolbar on zoom out mode? /cc @richtabor |
like a fixed toolbar? I'm not sure if I understand what this would entail |
Edit: this is exactly what happens in this PR now! |
scruffian
force-pushed
the
try-vertical-toolbar-zoomed-out
branch
from
d3a08b5
to
e2a6d60
Compare
|
I rebased this and now I'm seeing a few visual issues, sorry. I'm not sure if these are related to the rebase: 
|
scruffian
reviewed
Jun 18, 2024
|
|
||
| const { removeBlock } = useDispatch( blockEditorStore ); | ||
|
|
||
| const classNames = clsx( 'zoom-out-toolbar', { |
There was a problem hiding this comment.
I wonder if vertical-toolbar is a better name as this could be used in other context.
scruffian
reviewed
Jun 18, 2024
| @@ -14,6 +13,10 @@ | |||
| margin: 0; | |||
| } | |||
|
|
|||
| .components-accessible-toolbar:not(.zoom-out-toolbar) & { | |||
| background: $white; | |||
There was a problem hiding this comment.
I think it would be better to just remove this altogether, but there is a risk that it's used for something...
scruffian
reviewed
Jun 18, 2024
scruffian
reviewed
Jun 18, 2024
| @@ -20,6 +20,12 @@ | |||
|
|
|||
| .components-accessible-toolbar, | |||
| .components-toolbar { | |||
|
|
|||
| &[aria-orientation="vertical"] { | |||
There was a problem hiding this comment.
I moved this down so that it also targets components-toolbar as well as components-accessible-toolbar
|
I have fixed the issues above. Sometimes the toolbar ends up on the right, which is a bit surprising but it does work ok: |
scruffian
reviewed
Jun 19, 2024
There was a problem hiding this comment.
This is looking pretty good to me. My main concerns are:
- Should we rename zoom out toolbar to vertical toolbar or something more generic?
- Is there too much duplication in the
ZoomOutPopoverandZoomOutToolbarcomponents - would it be better to compose the base components?
I think the fact that is vertical is just an extra. The CSS for the component will be available to any other future vertical toolbars, the rest of the changes are specific to the needs of zoom out (mainly the contents of the toolbar we need it to have in this specific instance of it) |
MaggieCabrera
force-pushed
the
try-vertical-toolbar-zoomed-out
branch
from
b1b5d7a
to
81f4326
Compare
Co-authored-by: Lena Morita <[email protected]>
This reverts commit 59e5bdb.
MaggieCabrera
force-pushed
the
try-vertical-toolbar-zoomed-out
branch
from
b690f15
to
e1f50a0
Compare
carstingaxion
pushed a commit
to carstingaxion/gutenberg
that referenced
this pull request
Jul 18, 2024
* position toolbar popover to the left and change colors on zoomed-out mode * Make zoom out toolbar component * Create zoom out toolbar * Cleanup linting errors * Add orientation to toolbar component * css for vertical toolbars * Add similar classes to existing toolbar * Use correct placement for zoom out toolbar popover * remove cs that shouldn't be needed * Remove icon and block toolbar vertical css and apply just to base component * made toolbar unstyled and fixed stray background * Update Changelog * removed transparent background from component css * Update packages/components/src/toolbar/toolbar/types.ts Co-authored-by: Lena Morita <[email protected]> * document new prop for the toolbar component * remove white background * increase margin width * Also include the non-accessible toolbar in the vertical CSS rules * Move styles to the zoom out toolbar CSS * revert * avoid shifting of the toolbar to the right side * fix focus outline * moved vertical styles to toolbar component * missing display flex * removed orientation from NavigableToolbar props * Revert "removed orientation from NavigableToolbar props" This reverts commit 59e5bdb. * Use ToolbarButton instead of Button for Shuffle * added classes to avoid using component selectors --------- Co-authored-by: Jerry Jones <[email protected]> Co-authored-by: Lena Morita <[email protected]> Co-authored-by: Ben Dwyer <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Closes #59737
Ariakit supports vertical toolbars so we implemented a new variant to the main
Toolbarcomponent and applied it to a new toolbar specifically for the zoom-out mode.Why?
Zoom-out mode needs a toolbar that doesn't cover the content we are inserting. We needed to leverage the Ariakit toolbar for accessibility, so the black pseudo-toolbar that we were using wasn't enough.
How?
By implementing a toolbar that extends
NavigableToolbarand adding a new orientation prop that ariakit usesTesting Instructions
Open the inserter and open a pattern category
Click on a section, the toolbar should be visible on the side. All buttons should work as usual
Testing Instructions for Keyboard
When you focus on a section, move focus to the toolbar and use up and down arrows to navigate between the toolbar buttons
Screenshots or screencast